Skip to content

✨ Add Playwright options support to Ember SDK #171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Robdel12 merged 1 commit into from
Jan 6, 2026
Merged

✨ Add Playwright options support to Ember SDK #171

Robdel12 merged 1 commit into from
Jan 6, 2026

Conversation

@Robdel12
Copy link
Contributor

@Robdel12 Robdel12 commented Jan 5, 2026 •
edited
Loading

Summary

  • Rename vizzly-browser → vizzly-testem-launcher (clearer about its role)
  • Add playwrightOptions parameter to configure() for direct Playwright control
  • Default to headless mode (headless: true)
  • Support all Playwright browserType.launch() options
  • Simplify CI to Node 22 only

Usage

// Basic - headless by default
module.exports = configure({
  launch_in_ci: ['Chrome'],
});

// Headed for local debugging
module.exports = configure({
  launch_in_ci: ['Chrome'],
}, {
  headless: false,
  slowMo: 100,
});

// CI vs local
const isCI = process.env.CI;
module.exports = configure({
  launch_in_ci: ['Chrome'],
}, {
  headless: isCI,
});

Playwright Options

The second argument accepts Playwright browserType.launch() options directly:

Option Type Default Description
headless boolean true Run browser in headless mode
slowMo number - Slow down operations by specified ms
timeout number - Browser launch timeout
proxy object - Proxy settings
args string[] - Additional browser arguments

Test plan

  • Unit tests pass (38 tests)
  • Integration tests pass (5 tests)
  • Lint passes
  • CI passes

@claude
Copy link

claude bot commented Jan 5, 2026

I have reviewed PR #171. Overall this is a well-implemented feature with good test coverage. The implementation is clean and backwards compatible. I found a few minor suggestions for improvement that I will detail in a follow-up comment.

@claude
Copy link

claude bot commented Jan 5, 2026

Strengths

  1. Good Default Behavior - Defaulting to headless mode is the right choice for CI/CD environments. The opt-in approach for headed mode is clear and intuitive.

  2. Comprehensive Documentation - README updates are thorough with clear examples. API documentation properly describes the new options parameter.

  3. Solid Test Coverage - Unit tests verify both headless (default) and headed configurations. Tests properly check that the headless flag is added/omitted based on the option.

  4. Clean Implementation - Argument parsing correctly handles the headless flag. The configure() function signature is backwards compatible (second parameter is optional).

@claude
Copy link

claude bot commented Jan 5, 2026

Potential Issues and Suggestions

1. Argument Parsing Edge Case (Minor)

In bin/vizzly-browser.js line 28, the current filtering approach could theoretically filter out a URL that contains the browser type string. While unlikely in practice, consider using positional parsing since Testem always appends the URL as the final argument. This is a low-priority edge case.

2. Missing Test Case

The unit tests cover the launcher configuration, but there is no test verifying the actual argument parsing in vizzly-browser.js. Consider adding a unit test for the argument parsing logic.

3. Incomplete Test Plan

The PR description shows an unchecked integration testing item. Should this be completed before merging?

@claude
Copy link

claude bot commented Jan 5, 2026

Security and Performance

No concerns identified:

  • No security vulnerabilities introduced
  • No performance regressions (headless mode is actually faster)
  • No breaking changes (backwards compatible API)

Overall Assessment

This is a well-implemented feature that makes the Ember SDK more CI-friendly out of the box. The code quality is good, tests are comprehensive, and the API design is clean.

Recommendation: Approve with minor suggestions

The edge cases mentioned are low-priority and unlikely to occur in practice. The current implementation is production-ready if the integration testing checkbox is addressed.

Great work on this feature! The headless-by-default approach will save users configuration time and make CI setup smoother.

@Robdel12 Robdel12 changed the title ✨ Default to headless mode in Ember SDK ♻️ Refactor Ember SDK: Use Testem browser_args, expose Playwright options Jan 5, 2026
@blacksmith-sh

This comment has been minimized.

Sign in to view
@Robdel12
✨ Add Playwright options support to Ember SDK
6481931 
- Rename vizzly-browser → vizzly-testem-launcher (clearer purpose)
- Add playwrightOptions parameter to configure() for direct Playwright control
- Default to headless mode (headless: true)
- Support all Playwright browserType.launch() options (slowMo, timeout, proxy, etc.)
- Simplify CI to Node 22 only

Example usage:
```js
module.exports = configure({
  launch_in_ci: ['Chrome'],
}, {
  headless: process.env.CI,  // headed locally, headless in CI
  slowMo: 100,               // for debugging
});
```
@Robdel12 Robdel12 force-pushed the ember-sdk-headless-default branch from 63f6582 to 6481931 Compare January 5, 2026 23:19
@Robdel12 Robdel12 changed the title ♻️ Refactor Ember SDK: Use Testem browser_args, expose Playwright options ✨ Add Playwright options support to Ember SDK Jan 5, 2026
@Robdel12 Robdel12 merged commit a471365 into main Jan 6, 2026
22 of 31 checks passed
@Robdel12 Robdel12 deleted the ember-sdk-headless-default branch January 6, 2026 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Robdel12